Skip to content

[UEPR-518] Implement in-editor Manual Save Project Thumbnail logic#471

Open
kbangelov wants to merge 23 commits intoscratchfoundation:developfrom
kbangelov:task/uepr-518-manual-thumbnail-project-save-separately
Open

[UEPR-518] Implement in-editor Manual Save Project Thumbnail logic#471
kbangelov wants to merge 23 commits intoscratchfoundation:developfrom
kbangelov:task/uepr-518-manual-thumbnail-project-save-separately

Conversation

@kbangelov
Copy link
Copy Markdown
Contributor

@kbangelov kbangelov commented Mar 13, 2026

Resolves

https://scratchfoundation.atlassian.net/browse/UEPR-518

Proposed Changes

Moves the "update thumbnail" button from project page to project editor and adds:

  • a modal to confirm action
  • alerts for updating status, confirmation and fail message
  • blue loading circle

Design here : https://www.figma.com/design/nQK2PbAYG7pCmK4lYOTvHZ/Save-Thumbnail?node-id=370-1473&p=f&m=dev

To Do

  • show feature tooltip for moved button on first open of editor per user ONLY.

@kbangelov kbangelov requested a review from a team as a code owner March 13, 2026 12:27
@kbangelov kbangelov marked this pull request as draft March 13, 2026 12:28
Copy link
Copy Markdown
Contributor Author

@kbangelov kbangelov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have not changed delete confirmation prompt to use our new element since it has some visual differences and such a change has not been communicated with the Scratch team yet

@kbangelov kbangelov changed the title Implement in-editor Manual Save Project Thumbnail logic [UEPR-518] Implement in-editor Manual Save Project Thumbnail logic Mar 13, 2026
Copy link
Copy Markdown
Contributor

@KManolov3 KManolov3 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good job! Overall, comments are minor, but I do wonder if the logic in confirmation-prompt can be simplified.

border-radius: $form-radius;
}

[dir="ltr"] .stage-size-toggle-group {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Aren't those still used?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The design in the task description doesn't have that extra margin which would make the gaps uneven. Tereza agreed on 4px (.25 rem) for both gaps

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Even so, I believe they're still being referred in the code

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That was only needed previously when it was considered the leftmost and only element in that group. Now I add the gap from the parent component, so I removed the div entirely.

Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Implements the UEPR-518 UX for manually saving a project thumbnail from within the editor (instead of the project page), including a confirmation prompt and new alert states/styling.

Changes:

  • Move “Set Thumbnail” control into the in-editor Stage header and gate it by editor/project ownership state.
  • Add a reusable ConfirmationPrompt modal component with directional arrow assets and styling.
  • Add new thumbnail-related alerts (setting/success/error) and introduce a “blue” alert style.

Reviewed changes

Copilot reviewed 9 out of 15 changed files in this pull request and generated 11 comments.

Show a summary per file
File Description
packages/scratch-gui/src/lib/assets/icon--error.svg Adds an error icon asset used by the new thumbnail failure alert.
packages/scratch-gui/src/lib/alerts/index.jsx Adds new thumbnail alerts and introduces AlertLevels.BLUE.
packages/scratch-gui/src/components/stage-wrapper/stage-wrapper.jsx Threads editor/ownership props into StageHeader.
packages/scratch-gui/src/components/stage-header/stage-header.jsx Adds thumbnail button + confirmation flow and dispatches thumbnail alerts.
packages/scratch-gui/src/components/stage-header/stage-header.css Adjusts layout spacing for the stage size row.
packages/scratch-gui/src/components/stage-header/icon--thumbnail.svg Adds thumbnail button icon.
packages/scratch-gui/src/components/gui/gui.jsx Passes thumbnail/ownership/editor props into StageWrapper for editor mode.
packages/scratch-gui/src/components/confirmation-prompt/icon--arrow-up.svg Adds confirmation prompt arrow asset (up).
packages/scratch-gui/src/components/confirmation-prompt/icon--arrow-right.svg Adds confirmation prompt arrow asset (right).
packages/scratch-gui/src/components/confirmation-prompt/icon--arrow-left.svg Adds confirmation prompt arrow asset (left).
packages/scratch-gui/src/components/confirmation-prompt/icon--arrow-down.svg Adds confirmation prompt arrow asset (down).
packages/scratch-gui/src/components/confirmation-prompt/confirmation-prompt.jsx Introduces new confirmation prompt modal + positioning logic.
packages/scratch-gui/src/components/confirmation-prompt/confirmation-prompt.css Styles the confirmation prompt modal and buttons.
packages/scratch-gui/src/components/button/button.jsx Adds componentRef prop to expose the underlying button element ref.
packages/scratch-gui/src/components/alerts/alert.css Adds “blue” alert styling and adjusts alert box sizing.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

<ConfirmationPrompt
isOpen={isThumbnailPromptOpen}
title={messages.setThumbnail}
message={<FormattedMessage {...messages.setThumbnailMessage} />}
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The props being passed seem inconsistent here - strings in one place and FormattedMessage for the other props. I wonder if all strings makes more sense here?

width={336}
title={intl.formatMessage(messages.thumbnailTooltipTitle)}
body={
<FormattedMessage
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think I need to pass the node here because I don't see how else I'll be able to bold the text just inside the Tooltip component.

Copy link
Copy Markdown
Contributor

@KManolov3 KManolov3 Mar 20, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can consider intl.formatMessage as well

@kbangelov kbangelov marked this pull request as ready for review March 18, 2026 10:13
Comment on lines +30 to +34
const modalWidth = 200;
const spaceForArrow = 16;
const arrowOffsetFromEnd = 7;
const arrowLongSide = 29;
const arrowShortSide = 13;
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we truly want to make this component generic, we should make these configurable.

Comment on lines +244 to +254
{/* To remove - new feature awareness tooltip */}
<Tooltip
isOpen={isThumbnailTooltipOpen}
onRequestOpen={onOpenTooltip}
onRequestClose={onCloseTooltip}
targetRef={thumbnailButtonRef}
primaryPosition="left"
secondaryPosition="down"
width={336}
title={intl.formatMessage(messages.thumbnailTooltipTitle)}
body={
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The tooltip display is generally a part of another task - https://scratchfoundation.atlassian.net/browse/UEPR-522, so we don't need to handle it here. There are also more specific requirements that we need to implement, but I'd suggest leaving this as is for now and then finishing/updating the callout work as part of the other task.

Comment on lines +291 to +292
primaryPosition="down"
secondaryPosition="left"
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe rename these to side and align? I think it'd be easier for readers to understand which is which.
side - On which side of the anchor element should the prompt be displayed (top, bottom, left, right)
align - Where should the arrow be placed (left, center, right)

If we go with this approach, we'd also have to flip the logic for left and right for the secondary position. Right now, left indicates that most of the content is displayed on the left side of the button, but the arrow is displayed on the right side of the modal itself, which was not what I expected seeing this configuration.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can also see arrowPosition as an alternative naming for the align prop

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

since secondaryPosition is more about where the entire modal is placed (instead of centrally) I'll rename it to align

Comment on lines +33 to +46
switch (primaryPosition) {
case 'up':
top = buttonRect.top - modalHeight - spaceForArrow;
break;
case 'down':
top = buttonRect.bottom + spaceForArrow;
break;
case 'left':
left = buttonRect.left - popupWidth - spaceForArrow;
break;
case 'right':
left = buttonRect.right + spaceForArrow;
break;
}
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's add these values to an enum, instead of hardcoding them

const arrowLongSide = 29;
const arrowShortSide = 13;

const ConfirmationPrompt = ({
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we:

  • pass the positioning params (width, arrow sizes, ..) as a configuration object to the ConfirmationPrompt
  • reuse this component for the delete confirmation prompt, since it already contains a less complex version of the logic used here

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What I find slightly confusing about the current implementation is the following structure:

  • PopupWithArrow is a generic component used by ConfirmationPrompt, DeleteConfirmationPrompt, and FeatureCallout.
  • ConfirmationPrompt is also a generic component, but it's currently only used for the thumbnail confirmation prompt and not for the delete confirmation prompt.

I think what we actually need is a clearer separation of responsibilities:

  • SetTooltipConfirmationPrompt should use ConfirmationPrompt and pass in specific content such as title (if needed), description, className, layout, etc.
  • DeleteConfirmationPrompt should also use ConfirmationPrompt and provide its own specific content.
  • ConfirmationPrompt should use PopupWithArrow, which handles alignment and positioning.
  • FeatureCallout should use PopupWithArrow for alignment and positioning.
  • PopupWithArrow should use ReactModal to leverage its built-in behavior for handling outside clicks, escape key presses, etc.

This structure would simplify the overall design and ensure that each component has a clear, single responsibility. We may still need to refine the naming to better reflect each component's purpose, but this is the general direction I think we should take.

Comment on lines +291 to +292
primaryPosition="down"
secondaryPosition="left"
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can also see arrowPosition as an alternative naming for the align prop

@kbangelov kbangelov requested a review from Copilot March 19, 2026 11:30
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Implements “manual save project thumbnail” in the editor by moving the thumbnail action into the stage header UI and adding supporting UX (confirmation prompt, alerts, and new tooltip/popup positioning utilities).

Changes:

  • Add editor-stage “Set Thumbnail” button with confirmation prompt + (temporary) feature-awareness tooltip.
  • Add new alert types for thumbnail update progress/success/failure, including new styling/assets.
  • Introduce shared popup positioning helper and new UI components (Tooltip, ConfirmationPrompt) to support the new interactions.

Reviewed changes

Copilot reviewed 18 out of 28 changed files in this pull request and generated 13 comments.

Show a summary per file
File Description
packages/scratch-gui/src/reducers/project-state.js Adds getIsProjectLoadedWithId selector used to gate thumbnail UI behavior.
packages/scratch-gui/src/lib/store-project-thumbnail.js Refactors thumbnail snapshot/save flow to async/await and promise-based behavior.
packages/scratch-gui/src/lib/assets/icon--error.svg Adds error icon asset for new thumbnail-failure alert.
packages/scratch-gui/src/lib/alerts/index.jsx Adds new thumbnail-related alerts and a new INFO_BLUE alert level.
packages/scratch-gui/src/hooks/calculatePopupPosition.js Adds reusable popup/arrow positioning utility for tooltip/prompt UI.
packages/scratch-gui/src/css/colors.css Adds new alert-related color tokens.
packages/scratch-gui/src/containers/stage-header.jsx Wires redux state + alert dispatchers into the stage header.
packages/scratch-gui/src/components/tooltip/tooltip.jsx Adds new Tooltip component used for feature-awareness messaging.
packages/scratch-gui/src/components/tooltip/tooltip.css Adds styling for the new Tooltip component.
packages/scratch-gui/src/components/tooltip/icon--arrow-up.svg Adds tooltip arrow asset.
packages/scratch-gui/src/components/tooltip/icon--arrow-right.svg Adds tooltip arrow asset.
packages/scratch-gui/src/components/tooltip/icon--arrow-left.svg Adds tooltip arrow asset.
packages/scratch-gui/src/components/tooltip/icon--arrow-down.svg Adds tooltip arrow asset.
packages/scratch-gui/src/components/stage-wrapper/stage-wrapper.jsx Passes userOwnsProject down to stage header for gating thumbnail UI.
packages/scratch-gui/src/components/stage-header/stage-header.jsx Implements new “Set Thumbnail” button, confirmation prompt, tooltip, and alert-driven async flow.
packages/scratch-gui/src/components/stage-header/stage-header.css Updates layout and adds highlight styling for the new button/tooltip state.
packages/scratch-gui/src/components/stage-header/icon--thumbnail.svg Adds new thumbnail icon for the stage header button.
packages/scratch-gui/src/components/spinner/spinner.jsx Adds a default prop intended for spinner color customization.
packages/scratch-gui/src/components/spinner/spinner.css Adds styling for the new info-blue spinner level.
packages/scratch-gui/src/components/gui/gui.jsx Moves thumbnail props wiring to StageWrapper (instead of earlier location).
packages/scratch-gui/src/components/confirmation-prompt/icon--arrow-up.svg Adds confirmation prompt arrow asset.
packages/scratch-gui/src/components/confirmation-prompt/icon--arrow-right.svg Adds confirmation prompt arrow asset.
packages/scratch-gui/src/components/confirmation-prompt/icon--arrow-left.svg Adds confirmation prompt arrow asset.
packages/scratch-gui/src/components/confirmation-prompt/icon--arrow-down.svg Adds confirmation prompt arrow asset.
packages/scratch-gui/src/components/confirmation-prompt/confirmation-prompt.jsx Adds new ConfirmationPrompt component used to confirm thumbnail set action.
packages/scratch-gui/src/components/confirmation-prompt/confirmation-prompt.css Adds styling for the new ConfirmationPrompt component.
packages/scratch-gui/src/components/button/button.jsx Adds componentRef support for positioning tooltip/prompt relative to the button.
packages/scratch-gui/src/components/alerts/alert.css Adds new info-blue alert styling and updates warn styling to use shared tokens.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

log.error('Project thumbnail save error', e);
// This is intentionally fire/forget because a failure
// to save the thumbnail is not vitally important to the user.
throw e;
Comment on lines +126 to +133
await storeProjectThumbnail(vm, dataURI => new Promise((resolve, reject) => {
onUpdateProjectThumbnail(
projectId,
dataURItoBlob(dataURI),
resolve,
reject
);
}));
Comment on lines +12 to +23
export const getProjectThumbnail = (vm, callback) => new Promise((resolve, reject) => {
vm.postIOData('video', {forceTransparentPreview: true});
vm.renderer.requestSnapshot(dataURI => {
vm.postIOData('video', {forceTransparentPreview: false});
callback(dataURI);
const result = callback(dataURI);
result
.then(() => {
resolve();
})
.catch(e => {
reject(e instanceof Error ? e : new Error(String(e)));
});
@kbangelov kbangelov requested a review from Copilot March 19, 2026 12:36
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR implements in-editor manual “Set Thumbnail” functionality by moving the control into the project editor’s stage header and adding supporting UI/UX (confirmation prompt, new alerts, and updated spinner/alert styling).

Changes:

  • Adds a stage-header thumbnail button in the editor with a confirmation prompt and a “new feature” tooltip.
  • Introduces new alert types/styling for “setting thumbnail”, “success”, and “error” states.
  • Refactors thumbnail snapshot saving to be async-capable and exposes additional state selectors to gate UI.

Reviewed changes

Copilot reviewed 17 out of 27 changed files in this pull request and generated 4 comments.

Show a summary per file
File Description
packages/scratch-gui/src/reducers/project-state.js Adds getIsProjectLoadedWithId selector for gating the editor thumbnail UI.
packages/scratch-gui/src/lib/store-project-thumbnail.js Refactors thumbnail snapshot flow to async/Promise-based behavior.
packages/scratch-gui/src/lib/assets/icon--error.svg Adds an error icon asset for thumbnail failure alerts.
packages/scratch-gui/src/lib/alerts/index.jsx Adds new thumbnail alerts + introduces INFO_BLUE alert level.
packages/scratch-gui/src/hooks/calculatePopupPosition.js Adds a shared positioning helper used by tooltip/prompt popups.
packages/scratch-gui/src/css/colors.css Adds new alert-related color variables.
packages/scratch-gui/src/containers/stage-header.jsx Wires stage header to project load state + new alert dispatchers.
packages/scratch-gui/src/components/tooltip/tooltip.jsx Adds a custom tooltip component for the “new feature” callout.
packages/scratch-gui/src/components/tooltip/tooltip.css Styles for the new tooltip component.
packages/scratch-gui/src/components/tooltip/icon--arrow-up.svg Tooltip arrow asset.
packages/scratch-gui/src/components/tooltip/icon--arrow-right.svg Tooltip arrow asset.
packages/scratch-gui/src/components/tooltip/icon--arrow-left.svg Tooltip arrow asset.
packages/scratch-gui/src/components/tooltip/icon--arrow-down.svg Tooltip arrow asset.
packages/scratch-gui/src/components/stage-wrapper/stage-wrapper.jsx Passes userOwnsProject through to stage header.
packages/scratch-gui/src/components/stage-header/stage-header.jsx Moves/adds “Set Thumbnail” button, confirmation prompt, tooltip, and alert integration.
packages/scratch-gui/src/components/stage-header/stage-header.css Updates layout/styling for stage header control row.
packages/scratch-gui/src/components/stage-header/icon--thumbnail.svg Adds thumbnail button icon.
packages/scratch-gui/src/components/spinner/spinner.css Adds spinner styling for info-blue alert level.
packages/scratch-gui/src/components/gui/gui.jsx Adjusts prop wiring so stage wrapper receives manuallySaveThumbnails + userOwnsProject.
packages/scratch-gui/src/components/confirmation-prompt/icon--arrow-up.svg Confirmation prompt arrow asset.
packages/scratch-gui/src/components/confirmation-prompt/icon--arrow-right.svg Confirmation prompt arrow asset.
packages/scratch-gui/src/components/confirmation-prompt/icon--arrow-left.svg Confirmation prompt arrow asset.
packages/scratch-gui/src/components/confirmation-prompt/icon--arrow-down.svg Confirmation prompt arrow asset.
packages/scratch-gui/src/components/confirmation-prompt/confirmation-prompt.jsx Adds a reusable confirmation prompt component for the thumbnail action.
packages/scratch-gui/src/components/confirmation-prompt/confirmation-prompt.css Styles for the confirmation prompt.
packages/scratch-gui/src/components/button/button.jsx Adds componentRef support so consumers can attach refs to the underlying <button>.
packages/scratch-gui/src/components/alerts/alert.css Adds/updates alert styles for new levels and updated warning styling.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment on lines +126 to +133
await storeProjectThumbnail(vm, dataURI => new Promise((resolve, reject) => {
onUpdateProjectThumbnail(
projectId,
dataURItoBlob(dataURI),
resolve,
reject
);
}));
Comment on lines 3 to 9
export const storeProjectThumbnail = async (vm, callback) => {
try {
getProjectThumbnail(vm, callback);
await getProjectThumbnail(vm, callback);
} catch (e) {
log.error('Project thumbnail save error', e);
// This is intentionally fire/forget because a failure
// to save the thumbnail is not vitally important to the user.
throw e; // re-throw so it is handled by alert
}
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree - if we are updating this to be async, we should update all usages (so including the project-saver-hoc

}

.button-row button:focus {
outline: auto;
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this needed?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

currently yes because of the earlier all: unset; on the button

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if that's a sign not to use all:unset but rather change what actually needs to be changed? My concern is that we may unset some default button styles that we haven't though of testing. I don't have a strong preference though, that's just a thought.

width: 21rem;
padding: 1rem;
align-items: flex-start;
gap: -0.0625rem;
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are you sure a negative gap is what you want? Or do you indeed want to reduce paddings / margins?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is that way by design. I guess it's not terribly important whether we have it or not but I don't see a reason to diverge.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should overall try to avoid negative gaps/margins. It could be something that was added unintentionally (e.g. sometimes Figma adds certain styles when you move components). I think we should either remove it if we don't need it or try to achieve the same UI another way if possible.


const updatePosition = useCallback(() => {
if (!targetRef?.current || !tooltipRef.current) return;
const newPos = calculatePopupPosition({
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see a lot of similarities between the tooltip and the confirmation modal - the calls to calculatePopupPosition, the layout config, the arrowHeight / width. Do you see potential in extracting a common component between the two?

}, [isOpen, updatePosition]);

// Click outside to close
useEffect(() => {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Such behaviors seem like we are implementing parts of the standard <Modal behaviour. Is there any potential in using a modal component underneath?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps, but I believe that after bringing out the shared logic for PopupWithArrow, the component is simple enough.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1, most of these useEffects will become redundant if we switch to a Modal.

Comment on lines 3 to 9
export const storeProjectThumbnail = async (vm, callback) => {
try {
getProjectThumbnail(vm, callback);
await getProjectThumbnail(vm, callback);
} catch (e) {
log.error('Project thumbnail save error', e);
// This is intentionally fire/forget because a failure
// to save the thumbnail is not vitally important to the user.
throw e; // re-throw so it is handled by alert
}
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree - if we are updating this to be async, we should update all usages (so including the project-saver-hoc

@kbangelov kbangelov requested a review from KManolov3 March 25, 2026 10:24
}

.button-row button:focus {
outline: auto;
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if that's a sign not to use all:unset but rather change what actually needs to be changed? My concern is that we may unset some default button styles that we haven't though of testing. I don't have a strong preference though, that's just a thought.

});
try {
await storeProjectThumbnail(vm, dataURI => new Promise((resolve, reject) => {
onUpdateProjectThumbnail(
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, onUpdateProjectThumbnail already accepts onSuccess, onError. Can we not pass the callbacks directly? Why do we need to await the promises?

<FormattedMessage
{...messages.thumbnailTooltipBody}
values={{
boldText: <b>{intl.formatMessage(messages.setThumbnail)}</b>
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of using formatMessage inside a FormattedMessage, you can do something like what I've done in this PR. Basically that way you can simply add <b>...</b> in the message definition and that would bold it.

}, [isOpen, updatePosition]);

// Click outside to close
useEffect(() => {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1, most of these useEffects will become redundant if we switch to a Modal.

@kbangelov kbangelov requested a review from adzhindzhi March 25, 2026 13:11
@kbangelov kbangelov requested a review from KManolov3 March 26, 2026 10:40
} = {...defaultConfig, ...layoutConfig};

const memoizedLayoutConfig = React.useMemo(() => ({
popupWidth: modalWidth,
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nitpick: Can we use either modalWidth or popupWidth everywhere for consistency?

const arrowLongSide = 29;
const arrowShortSide = 13;

const ConfirmationPrompt = ({
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What I find slightly confusing about the current implementation is the following structure:

  • PopupWithArrow is a generic component used by ConfirmationPrompt, DeleteConfirmationPrompt, and FeatureCallout.
  • ConfirmationPrompt is also a generic component, but it's currently only used for the thumbnail confirmation prompt and not for the delete confirmation prompt.

I think what we actually need is a clearer separation of responsibilities:

  • SetTooltipConfirmationPrompt should use ConfirmationPrompt and pass in specific content such as title (if needed), description, className, layout, etc.
  • DeleteConfirmationPrompt should also use ConfirmationPrompt and provide its own specific content.
  • ConfirmationPrompt should use PopupWithArrow, which handles alignment and positioning.
  • FeatureCallout should use PopupWithArrow for alignment and positioning.
  • PopupWithArrow should use ReactModal to leverage its built-in behavior for handling outside clicks, escape key presses, etc.

This structure would simplify the overall design and ensure that each component has a clear, single responsibility. We may still need to refine the naming to better reflect each component's purpose, but this is the general direction I think we should take.

arrowRightIcon
};

const BUTTON_ORDER = {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ideally, I'd generalize the props to:

`mainButtonIcon`
`mainButtonStyles`
`mainButtonLabel`
`mainButtonAction`
`secondaryButtonIcon`
`secondaryButtonStyles`
`secondaryButtonLabel`
`secondaryButtonAction`

But I'm not insisting us do that here, as the confirm/cancel separation fits the current cases

message,
confirmLabel,
cancelLabel,
confirmIcon,
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we group the related props in a confirmButtonConfig, canceButtonConfig objects?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Or alternatively, we can pass primaryButtonContent/secondaryButtonContent - a React node, which can either be the text if that's all we want to display, or text + icon. Both seem fine to me, whatever you feel is better.

isOpen
onRequestClose={onCancel}
title={intl.formatMessage(messages.confirmDeletionHeading)}
message={<FormattedMessage {...getMessage(entityType)} />}
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks strange to use FormattedMessage for the message and intl.formatMessage for the title. Any reason to have it differ?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, in ConfirmationPrompt I use css styles that I apply to spans and FormattedMessage renders one. And title is just passed purely for accessibility.

margin: auto;
}

.button-row button.ok-button {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Feelsgood to remove this code

@kbangelov kbangelov requested a review from KManolov3 March 27, 2026 13:13
className={styles.buttonIcon}
src={confirmIcon}
aria-hidden="true"
alt=""
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nitpick: Maybe we shouldn't leave the alt text empty, since we are also working on making the editor more accessible? Same for the cancel button?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since the images are not supposed to be announced, I'll just remove the alt props alltogether.

Comment on lines +116 to +132
try {
storeProjectThumbnail(vm, dataURI => {
onUpdateProjectThumbnail(projectId, dataURItoBlob(dataURI));
onUpdateProjectThumbnail(
projectId,
dataURItoBlob(dataURI),
() => {
onShowThumbnailSuccess();
setIsUpdatingThumbnail(false);
},
() => {
onShowThumbnailError();
setIsUpdatingThumbnail(false);
}
);
});
},
3000
),
[projectId, onUpdateProjectThumbnail]
} catch (e) {
onShowThumbnailError();
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would we ever get to the catch of this try/catch block? If the thumbnail update fails, it'll call the onError callback, but shouldn't fire an error that we can can here? The storeProjectThumbnail has its own try/catch block, which swallows the error, so it wouldn't bubble up to here?

@kbangelov kbangelov requested a review from adzhindzhi March 27, 2026 14:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants